Skip to content

refactor: Remove pzip plugin and update CMake configuration#353

Merged
deepin-bot[bot] merged 1 commit intolinuxdeepin:develop/snipefrom
dengzhongyuan365-dev:pzip-cpp
Jan 20, 2026
Merged

refactor: Remove pzip plugin and update CMake configuration#353
deepin-bot[bot] merged 1 commit intolinuxdeepin:develop/snipefrom
dengzhongyuan365-dev:pzip-cpp

Conversation

@dengzhongyuan365-dev
Copy link
Contributor

@dengzhongyuan365-dev dengzhongyuan365-dev commented Jan 20, 2026

  • Deleted the pzip plugin directory and its associated files, including CMakeLists.txt, source, and header files.
  • Updated CMakeLists.txt to reflect the removal of the pzip plugin, ensuring proper build configuration without it.
  • Adjusted the add_subdirectory command for pzip to use the correct source and binary directories.

Log: Clean up project structure by removing the pzip plugin and updating build configurations accordingly.

Summary by Sourcery

Update CMake configuration to relocate and gate the pzip-related build logic under src while simplifying third-party plugin setup.

Build:

  • Move the conditional pzip subdirectory inclusion from 3rdparty to src, enabling it only on x86/ARM with Qt6.
  • Adjust 3rdparty CMake configuration to only include the clipzipplugin under the same platform/Qt6 conditions and update its descriptive comments.

@sourcery-ai
Copy link

sourcery-ai bot commented Jan 20, 2026

Reviewer's guide (collapsed on small PRs)

Reviewer's Guide

Refactors the build configuration to move the pzip-dependent logic from 3rdparty into src, while keeping clipzipplugin gated behind the same architecture/Qt6 conditions and updating comments/messages accordingly.

Flow diagram for conditional enabling of pzip and clipzipplugin

flowchart TD
    A[Configure CMake] --> B{Architecture is aarch64 or arm or x86_64 or amd64?}
    B -->|no| C[Build without pzip and clipzipplugin]
    B -->|yes| D{QT_VERSION_MAJOR equals 6?}
    D -->|no| C
    D -->|yes| E[Enable src pzip via add_subdirectory in src/CMakeLists]
    E --> F[Enable clipzipplugin via add_subdirectory in 3rdparty/CMakeLists]
Loading

File-Level Changes

Change Details Files
Rehome pzip CMake integration under src and gate it by architecture/Qt6.
  • Add a conditional block in src/CMakeLists.txt that checks CPU architecture and Qt major version.
  • Invoke add_subdirectory(pzip) only when on aarch64/arm/x86_64/amd64 and Qt 6.
  • Document in comments that pzip is a high‑performance parallel compression tool enabled only on supported platforms.
src/CMakeLists.txt
Decouple 3rdparty CMake from pzip and clarify clipzipplugin’s dependency on src/pzip.
  • Update the comment above the conditional to describe clipzipplugin and its dependency on src/pzip instead of pzip directly.
  • Remove add_subdirectory(pzip) from the 3rdparty build flow.
  • Keep clipzipplugin behind the same architecture/Qt6 condition and preserve status messages for enabled/disabled paths.
3rdparty/CMakeLists.txt

Tips and commands

Interacting with Sourcery

  • Trigger a new review: Comment @sourcery-ai review on the pull request.
  • Continue discussions: Reply directly to Sourcery's review comments.
  • Generate a GitHub issue from a review comment: Ask Sourcery to create an
    issue from a review comment by replying to it. You can also reply to a
    review comment with @sourcery-ai issue to create an issue from it.
  • Generate a pull request title: Write @sourcery-ai anywhere in the pull
    request title to generate a title at any time. You can also comment
    @sourcery-ai title on the pull request to (re-)generate the title at any time.
  • Generate a pull request summary: Write @sourcery-ai summary anywhere in
    the pull request body to generate a PR summary at any time exactly where you
    want it. You can also comment @sourcery-ai summary on the pull request to
    (re-)generate the summary at any time.
  • Generate reviewer's guide: Comment @sourcery-ai guide on the pull
    request to (re-)generate the reviewer's guide at any time.
  • Resolve all Sourcery comments: Comment @sourcery-ai resolve on the
    pull request to resolve all Sourcery comments. Useful if you've already
    addressed all the comments and don't want to see them anymore.
  • Dismiss all Sourcery reviews: Comment @sourcery-ai dismiss on the pull
    request to dismiss all existing Sourcery reviews. Especially useful if you
    want to start fresh with a new review - don't forget to comment
    @sourcery-ai review to trigger a new review!

Customizing Your Experience

Access your dashboard to:

  • Enable or disable review features such as the Sourcery-generated pull request
    summary, the reviewer's guide, and others.
  • Change the review language.
  • Add, remove or edit custom review instructions.
  • Adjust other review settings.

Getting Help

Copy link

@sourcery-ai sourcery-ai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hey - I've left some high level feedback:

  • The PR description mentions removing the pzip plugin, but src/CMakeLists.txt now adds add_subdirectory(pzip)—please reconcile the description with the actual change and ensure the src/pzip directory and build targets exist as expected.
  • In 3rdparty/CMakeLists.txt, the status message still says it is enabling the "pzip 高性能压缩插件" even though only clipzipplugin is added now; consider updating the message and the new comment about depending on src/pzip to clearly reflect the new structure and dependency direction.
Prompt for AI Agents
Please address the comments from this code review:

## Overall Comments
- The PR description mentions removing the pzip plugin, but `src/CMakeLists.txt` now adds `add_subdirectory(pzip)`—please reconcile the description with the actual change and ensure the `src/pzip` directory and build targets exist as expected.
- In `3rdparty/CMakeLists.txt`, the status message still says it is enabling the "pzip 高性能压缩插件" even though only `clipzipplugin` is added now; consider updating the message and the new comment about depending on `src/pzip` to clearly reflect the new structure and dependency direction.

Sourcery is free for open source - if you like our reviews please consider sharing them ✨
Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.

@deepin-ci-robot
Copy link

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: dengzhongyuan365-dev, lzwind

The full list of commands accepted by this bot can be found here.

Details Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@dengzhongyuan365-dev
Copy link
Contributor Author

/forecemerge

@deepin-ci-robot
Copy link

deepin pr auto review

经过对代码差异的审查,这次变更主要是将 pzip 及其相关文件从 3rdparty 目录移动到 src 目录,并更新了构建系统(CMake)和运行时路径查找逻辑。

以下是从语法逻辑、代码质量、代码性能和代码安全四个方面的详细审查意见:

1. 语法逻辑

  • CMake 构建逻辑

    • 变更点:在 3rdparty/CMakeLists.txt 中移除了 add_subdirectory(pzip),而在 src/CMakeLists.txt 中添加了它。条件判断保持一致。
    • 意见:逻辑正确。将项目源码从 3rdparty(通常用于存放外部依赖)移动到 src(通常用于存放项目自身源码)是合理的,说明 pzip 现在被视为项目核心组件的一部分。
    • 注意:请确保 src/pzip/CMakeLists.txt 中的目标名称、头文件包含路径等配置已经适应了新的目录位置。如果原 CMake 中使用了相对路径引用父目录(如 ${CMAKE_CURRENT_SOURCE_DIR}/../common),这些路径可能需要更新。
  • 运行时路径查找逻辑

    • 变更点:在 clipzipplugin.cpp 中,将开发环境下的查找路径从 appDir + "/../3rdparty/pzip/pzip" 修改为 appDir + "/../src/pzip/pzip"
    • 意见:逻辑正确。这与代码目录结构的移动保持一致,确保了插件在开发环境下能够正确找到可执行文件。

2. 代码质量

  • 硬编码路径

    • 问题:在 clipzipplugin.cpp 中,路径字符串是硬编码拼接的(例如 appDir + "/../src/pzip/pzip")。
    • 改进建议
      • 虽然目前的改动是必要的,但使用字符串拼接构建路径存在跨平台兼容性问题(例如 Windows 使用反斜杠)。
      • 建议使用 Qt 的 QDir 类来处理路径拼接,这样更健壮且可读性更好。
      • 示例代码
        #include <QDir>
        // ...
        QString devPath = QDir(QCoreApplication::applicationDirPath())
                              .filePath("../src/pzip/pzip");
  • 注释更新

    • 意见3rdparty/CMakeLists.txt 中的注释更新为 clipzipplugin(仅 x86/ARM + Qt6 环境启用,依赖 src/pzip),这准确地反映了当前的依赖关系,做得很好。

3. 代码性能

  • 文件系统访问
    • 现状QFileInfo::exists(devPath) 会进行磁盘 I/O 操作。
    • 意见:由于这是在插件初始化或调用时执行的,且仅执行一次,对性能影响微乎其微。无需优化。

4. 代码安全

  • 路径遍历风险

    • 风险分析:代码中使用了 ../ 进行相对路径回退。虽然这是为了在开发环境中定位可执行文件,但如果 QCoreApplication::applicationDirPath() 返回的路径受到外部控制或不可信(在常规桌面应用中较少见),理论上存在路径遍历的风险。
    • 结论:在当前上下文(桌面压缩工具)中,这是可接受的开发环境适配逻辑,安全风险较低。
  • 依赖关系

    • 观察clipzipplugin 现在依赖于 src/pzip。这建立了一个内部依赖。
    • 建议:确保在最终打包和安装阶段,pzip 的可执行文件被正确安装到了系统路径(如 /usr/bin)或应用指定的特定目录,并且 clipzipplugin 能够优先找到安装后的版本,而不是依赖开发环境的相对路径。目前的代码逻辑(先检查 appDir,再检查开发路径)似乎已经考虑到了这一点,但需确认安装路径的优先级是否足够高。

总结与改进建议

这次代码重构主要是目录结构的调整,逻辑上是自洽的。为了提高代码的健壮性和可维护性,建议采纳以下改进:

  1. 使用 QDir 处理路径:修改 clipzipplugin.cpp 中的路径拼接方式,使用 QDir::filePath()QDir::cleanPath() 来保证跨平台兼容性和路径格式的规范性。

  2. 统一宏定义(可选):考虑到 x86/ARM + Qt6 的判断条件在 3rdparty/CMakeLists.txtsrc/CMakeLists.txt 中重复出现,如果未来条件可能变化,可以考虑在顶层 CMakeLists.txt 中定义一个宏(例如 ENABLE_PZIP_PLUGIN),然后在子 CMake 中直接判断该宏,减少重复代码。

    # 在顶层 CMakeLists.txt
    set(ENABLE_PZIP_PLUGIN FALSE)
    if((CMAKE_SYSTEM_PROCESSOR MATCHES "aarch64|arm|x86_64|amd64") AND (QT_VERSION_MAJOR EQUAL 6))
        set(ENABLE_PZIP_PLUGIN TRUE)
    endif()
    
    # 在子目录中
    if(ENABLE_PZIP_PLUGIN)
        add_subdirectory(...)
    endif()
  3. 确认 CMake 路径:请务必检查移动后的 src/pzip/CMakeLists.txt,确保没有遗留的指向 ../3rdparty/... 的路径引用,否则会导致构建失败。

- Deleted the pzip plugin directory and its associated files, including CMakeLists.txt, source, and header files.
- Updated CMakeLists.txt to reflect the removal of the pzip plugin, ensuring proper build configuration without it.
- Adjusted the add_subdirectory command for pzip to use the correct source and binary directories.

Log: Clean up project structure by removing the pzip plugin and updating build configurations accordingly.
@dengzhongyuan365-dev
Copy link
Contributor Author

/forcemerge

@deepin-bot
Copy link
Contributor

deepin-bot bot commented Jan 20, 2026

This pr force merged! (status: unstable)

@deepin-bot deepin-bot bot merged commit b00add3 into linuxdeepin:develop/snipe Jan 20, 2026
14 of 15 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants